Skip to content

fix: reject update payloads the padding would silently misread - #1

Closed
tschm wants to merge 1 commit into
mainfrom
fix/update-dimension-consistency
Closed

fix: reject update payloads the padding would silently misread#1
tschm wants to merge 1 commit into
mainfrom
fix/update-dimension-consistency

Conversation

@tschm

@tschm tschm commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Why

Model.update zero-pads short input up to the compiled size — that is what lets one compiled problem serve a universe smaller than the one it was built for. Two things went wrong at the edges of it.

1. Models disagreeing about the universe solved, and solved wrongly. Give the risk model two assets and the bounds four, and the padded tail carries no risk while the bounds leave it free. The solver reads that as riskless assets and puts the portfolio there:

MinVar(assets=4), 2-asset chol + 4-asset bounds
-> value 2.4e-09, weights [6e-09, 6e-09, 0.5, 0.5]

No exception, no bad status. No single model can see it either — each one's own inputs are internally consistent.

2. Oversized input escaped the CvxError tree. Handing update more assets than were compiled gave numpy's ValueError: could not broadcast input array from shape (5,5) into shape (4,4), which names neither the caller's mistake nor a class the README's error table promises.

What changed

  • Model.dimensions — new abstract method returning the (variable name, size) claims a payload implies, keyed by DataNames.WEIGHTS / FACTOR_WEIGHTS. Implemented by all seven concrete models.
  • Problem.update delegates to a new _validate that runs keyword presence and dimension agreement over every model before the first value is written. A conflict raises CvxDataError:
    Inconsistent size for weights: model risk was given 2, model bound_assets was given 4
  • fill_vector / fill_matrix raise CvxDataError for input that does not fit. Padding stays one-directional — truncating would drop assets the caller asked about.
  • Bounds.update's docstring claimed to trim, which the helper never did.
  • README and CLAUDE.md record the invariant.

Two design notes:

  • Abstract, not defaulted. keywords uses the defaulted-with-override-obligation shape, and that contract has already been got wrong once here (the mu_uncertainty bug its docstring describes). Abstract makes the omission impossible rather than documented. This is breaking for any external Model subclass; nothing in the repo or experiments/ subclasses it.
  • Pairs, not a mapping. A model declares several inputs against the same variable, so Bounds declares both bounds — otherwise a short lower bound against a full-length upper bound reproduces the same riskless-tail bug from inside one model. One merge pass catches intra- and inter-model disagreement.

The existing per-model shape checks are untouched: they are what a model gives you when used directly, and their tests call model.update rather than going through Problem.

Tests

  • test_aux.py — exact-fit and too-large cases for both helpers, parametrized over rows/cols/both. Also added the missing assert to the two existing tests, which called np.allclose(...) and discarded the result.
  • test_problem.py — the phantom-asset payload is rejected and the message names both models; Bounds contradicting itself is rejected; factor counts are checked as their own dimension; a legitimate padded universe still solves to the exact-size answer with a zero tail; a rejected payload leaves the problem solving to its previous value; an oversized payload surfaces as CvxDataError through update.
  • test_holding_costs.py / test_trading_costs.py — direct dimensions tests; those two models are never assembled into a Builder.

make all is green: 104 tests, 100% test and docstring coverage, ty + mypy --strict clean, no template-owned file modified.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added comprehensive validation for input dimensions across portfolio models.
    • Shorter inputs continue to be zero-padded to the compiled problem size.
    • Updates now fail safely when inputs are oversized or models disagree on dimensions.
    • Invalid updates are rejected before any existing values are changed.
  • Documentation

    • Documented input sizing, padding behavior, and validation rules.
  • Tests

    • Added coverage for dimension reporting, mismatches, oversized inputs, and atomic update behavior.

`Model.update` zero-pads short input up to the compiled size, which is what
lets one compiled problem serve a smaller universe. Two ways that went wrong:

A payload whose models disagreed about the universe size solved without
complaint and answered with nonsense. Giving the risk model two assets and the
bounds four leaves the padded tail carrying no risk while the bounds leave it
free, which the solver reads as riskless assets and fills:

    MinVar(assets=4), 2-asset chol + 4-asset bounds
    -> value 2.4e-09, weights [6e-09, 6e-09, 0.5, 0.5]

No single model can see this; each one's own inputs are consistent. `Model`
now declares `dimensions`, the (variable, size) claims its inputs imply, and
`Problem.update` collects them across all models and rejects a disagreement
with `CvxDataError`. Both bounds are declared, so a model contradicting itself
is caught by the same pass. Validation now runs over every model before the
first value is written, so a rejected payload leaves the problem untouched
rather than half-overwritten.

`dimensions` is abstract rather than a default a model may quietly not
override: `keywords` has that shape and it has already been got wrong once.

Second, input larger than the compiled problem escaped as numpy's broadcast
`ValueError`, outside the `CvxError` tree the README promises. `fill_vector`
and `fill_matrix` now raise `CvxDataError`; padding stays one-directional, as
truncating would drop assets the caller asked about. `Bounds.update` claimed
to trim in its docstring, which the helper never did.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: c42d9318-85c8-4ce4-a0f0-b5ecf79d0841

📥 Commits

Reviewing files that changed from the base of the PR and between 22138ae and f7dd73e.

📒 Files selected for processing (17)
  • CLAUDE.md
  • README.md
  • src/cvxmarkowitz/model.py
  • src/cvxmarkowitz/models/bounds.py
  • src/cvxmarkowitz/models/expected_returns.py
  • src/cvxmarkowitz/models/holding_costs.py
  • src/cvxmarkowitz/models/trading_costs.py
  • src/cvxmarkowitz/problem.py
  • src/cvxmarkowitz/risk/cvar/cvar.py
  • src/cvxmarkowitz/risk/factor/factor.py
  • src/cvxmarkowitz/risk/sample/sample.py
  • src/cvxmarkowitz/types.py
  • src/cvxmarkowitz/utils/fill.py
  • tests/test_markowitz/test_models/test_holding_costs.py
  • tests/test_markowitz/test_models/test_trading_costs.py
  • tests/test_markowitz/test_portfolios/test_problem.py
  • tests/test_markowitz/test_utils/test_aux.py

📝 Walkthrough

Walkthrough

The change adds a Model.dimensions contract, implements dimension reporting across models, validates all payloads before updates, rejects oversized inputs, and adds tests and documentation for padding, consistency, and atomicity.

Changes

Dimension validation

Layer / File(s) Summary
Dimension declaration contract
src/cvxmarkowitz/types.py, src/cvxmarkowitz/model.py, src/cvxmarkowitz/models/*, src/cvxmarkowitz/risk/*
Models now report input-derived variable dimensions through Model.dimensions.
Pre-write payload validation
src/cvxmarkowitz/problem.py, src/cvxmarkowitz/utils/fill.py
Problem.update validates required keywords and cross-model dimensions before mutation. Fill utilities reject oversized vectors and matrices with CvxDataError.
Validation regression coverage
tests/test_markowitz/test_models/*, tests/test_markowitz/test_portfolios/test_problem.py, tests/test_markowitz/test_utils/test_aux.py, README.md, CLAUDE.md
Tests and documentation cover padding, dimension mismatches, atomic updates, oversized inputs, and dimension declarations.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Problem
  participant Models
  participant FillUtilities
  Client->>Problem: submit update payload
  Problem->>Models: collect dimensions and validate keywords
  Problem->>FillUtilities: validate vector and matrix sizes
  Problem->>Models: apply update after validation
  Models-->>Client: updated problem state
Loading
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/update-dimension-consistency

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@tschm

tschm commented Sep 2, 2026

Copy link
Copy Markdown
Owner Author

Retargeted to the upstream repo: cvxgrp#607. Same branch, same commit.

@tschm tschm closed this Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant